fix(contracts): skip cleanup_pubkey_mapping when signer count does n…#259
fix(contracts): skip cleanup_pubkey_mapping when signer count does n…#259zeljkoX wants to merge 2 commits into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughBoth multisig and multisig_ecdsa contracts now conditionally execute pubkey map cleanup based on approver count comparison. The test is updated to make the mock chain mutable and properly advance block state after transaction execution before validating storage changes. ChangesConditional Cleanup in Multisig Contracts
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a STARK proof verification failure in the multisig contracts by avoiding entering cleanup_pubkey_mapping unless the signer set actually shrinks, and strengthens the regression test to exercise block proving.
Changes:
- Gate
cleanup_pubkey_mappingbehind ainit_num_of_approvers > new_num_of_approverscheck in both Falcon and ECDSA multisig MASM. - Extend the “add signer from single signer” test to add the executed tx to
MockChainand callprove_next_block()to ensure proof generation is covered.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/contracts/tests/auth/multisig.rs | Extends add-signer test to prove the next block (covers proof generation path). |
| crates/contracts/masm/auth/multisig.masm | Conditionally calls cleanup_pubkey_mapping only when approver count decreases. |
| crates/contracts/masm/auth/multisig_ecdsa.masm | Same conditional cleanup gating as Falcon variant for parity. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #259 +/- ##
==========================================
+ Coverage 75.28% 76.37% +1.09%
==========================================
Files 131 136 +5
Lines 23314 24888 +1574
==========================================
+ Hits 17551 19009 +1458
- Misses 5763 5879 +116 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Calling
exec.cleanup_pubkey_mappingafterupdate_signers_and_thresholdunconditionally produces a STARK-invalid proof when the storage map has
exactly one entry (e.g. add-signer from a 1-signer account). The proc's
internal loop guard already short-circuits in this case, so the body never
ran — but entering the proc at all perturbs the prover trace enough to fail
node-side STARK verification with
constraint mismatch: quotient * vanishing != folded constraints.Gate the call on
init_num_of_approvers > new_num_of_approversso cleanuponly runs when the signer set actually shrinks. Add-signer and threshold-only
updates skip the call entirely; remove-signer behavior is unchanged.
Applied to both Falcon and ECDSA multisig MASM. The existing add-signer test
is extended to call
prove_next_block()so MockChain exercises proofgeneration and would catch this class of regression.
TODO: Open PR against https://github.com/OpenZeppelin/miden-confidential-contracts and verify with @onurinanc
Summary by CodeRabbit
Refactor
Tests